-
-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General type inference improvements #98
Conversation
- Adds template parameters to ParameterObjectInterface and AbstractOptions - Fixes a range of psalm issues, mostly in tests - Updates the baseline - Bumps dev dependencies and refreshes the lock file Signed-off-by: George Steel <george@net-glue.co.uk>
Thanks @gsteel! |
I like that the generic is just /**
* @psalm-type WhateverOptionsArrayShape = array{key1: bool, key2: int}
* @template-extends AbstractOptions<WhateverOptionsArrayShape>
*/
final class WhateverOptions extends AbstractOptions
{
public function setKey1(bool $value): void
{}
public function getKey1(): bool
{}
// etc
} Sadly, that does not work, since it is not possible. /**
* @template TValue of iterable
* @implements ParameterObjectInterface<TValue>
*/
abstract class AbstractOptions
{
/**
* @param TValue|AbstractOptions<TValue>|null $options
*/
public function __construct($options = null)
{}
} Just an idea, I've added these types via stubs for now in our projects since that suits for us better but will likely diverge with other component implementations which would be sad. |
Using a shape as a template would only work if the implementation used an array internally for storage right? i.e. /**
* @template TShape = array{foo: string}
*/
abstract class AbstractOptions {
/** @var TShape */
protected array $storage;
/** @param TShape|self<TShape> $options */
public function __construct(iterable $options) {
$options = ArrayUtils::iterableToArray($options);
$this->storage = $options;
}
public function setFoo(string $foo): void
{
$this->storage['foo'] = $foo;
}
public function getFoo(): string
{
return $this->storage['foo'];
}
} Or am I missing something? It was a while ago I did this, but IIRC, using a shape wasn't possible because psalm can't translate the keys to properties declared in the concrete class (??) |
Actually not. In this case, array keys to be passed have to reflect the property structure. What you could do with psalm is something like: /**
* @psalm-type MyFancyOptionsType = array<property-of<MyFancyOptions>,mixed>
*/ But obviously, that won't be really helpful. /**
* @psalm-type MyFancyOptionsType = array{property1:int,property2:bool}
*/ As this will tell psalm exactly what the array must contain. Thats why I would not accept I'd only allow an array shape which allows upstream to explicitly declare what stuff is required and what is optional (i.e. by adding IMHO that would be a huge benefit for this class. |
Thanks for explaining @boesing - yes, accepting only an array shape to I'm still not quite sure I'm getting everything completely, because having had a look at various patches in psalm, we could actually use At some point, I'll look into this in more detail to improve my understanding of it. Cheers |
But it is not just property-of since it has to be a map of values which are then passed to the properties declared in the implementing class. Therefore, that example you state wont fit the requirement. |
Description
Closes #97
Primarily, this patch is just to get renovate green again because psalm has made everything go red with various improvements.
Stdlib is also good to go for PHP 8.3, so I'll send in another patch for this.